feat: construct BTC transactions #128

djordon · 2024-05-06T21:50:57Z

Description

Closes #75.

We need to take a set of deposit and withdrawal requests and turn them into BTC transactions that can be signed by the signers. This PR gets us part of the way there.

Type of Change

This is new code that takes the current state of the signers UTXO, the current market rate in sats-per-vbyte, along with outstanding deposit and withdrawal requests and creates a set of BTC transactions.

Notes

There are several things worth noting though:

The UTXO output scripts for withdrawals are pay-to-witness-public-key-hash (P2WPKH) outputs. I think this is fine, since it's probably okay to a assume our users have native segwit addresses but I don't know about assuming the same for taproot addresses. In the future, maybe the user can decide?
I have not implemented filtering of requests based on their max fee and the current market fee rate.
There is nothing here that accommodates things related to replace-by-fee (RBF) transactions. That stuff will get done in another PR addressing Add functionality to transaction package construction to support RBF #66.
I'm not sure what the final API here will be, the public stuff here is not close to final.

Does this introduce a breaking change?

No breaking changes

Testing information

There are unit tests here that test for most of the asserted functionality and/or invariants. But some further integration testing is required to ensure that properly signed witness data is indeed correct. These tests will happen in another PR (such as one addressing #67).

Checklist

Code is commented where needed
Unit test coverage for new or modified code paths
Tag reviewers

netrome

Great stuff so far. The only blocker I see is that we shouldn't dictate the output type for withdrawals.

netrome · 2024-05-07T08:43:38Z

signer/src/utxo.rs

+    /// The public key that will be able to spend the output.
+    pub public_key: PublicKey,


I don't think we should assume a public key in the withdraw request. Perhaps the user wants to withdraw to a multisig address? Or another script?

Instead, I think this should be an Address, alternatively a ScriptBuf representing the output scriptPubKey if we want to be more low-level here.

Suggested change

/// The public key that will be able to spend the output.

pub public_key: PublicKey,

/// The public key that will be able to spend the output.

pub public_key: bitcoin::address::Address,

That all makes sense to me, but I think that would lead us being off the low-level designs in the Deposit API LLD and the Clarity LLD. I was following the Deposit API LLD, which uses the public key, while the clarity LLD uses a tuple of a 1 byte version field with a 32 byte hashbytes field (which I think is a compressed public key).

I'm okay changing the designs and it's all equally easy for me to update here. Just want to call out that this would lead to a mismatch.

(for reference) We've synced with the team and agreed that this is a LLD bug, and that we want to use addresses here.

netrome · 2024-05-07T08:50:36Z

signer/src/utxo.rs

+    /// The maximum acceptable number of votes against for any given
+    /// request.
+    pub reject_capacity: u32,


Nit: It's more intuitive for me to work with the converse: accept_threshold. That's the parameter that we already use in many places in wsts and the Nakamoto signer (just called threshold there). Would you be okay with swapping it here?

Suggested change

/// The maximum acceptable number of votes against for any given

/// request.

pub reject_capacity: u32,

/// The minimum acceptable number of votes for any given request.

pub threshold: u32,

Yeah, it is more intuitive. I think I had that design originally but with an additional field noting the total number of signers so that we could compute the reject capacity (the accept threshold alone is not enough). I'm okay with switching to that version of things with the additional field.

netrome · 2024-05-07T08:58:14Z

signer/src/utxo.rs

+    /// The public key used in the deposit script.
+    pub deposit_public_key: PublicKey,


Which public key is this? There should only be one public key in the deposit script, and that's the signer aggregate key.

Right, so this should be the aggregate key. Can we rename it?

Suggested change

/// The public key used in the deposit script.

pub deposit_public_key: PublicKey,

/// The public key used in the deposit script.

pub signer_aggregate_key: PublicKey,

That makes sense. Are you opposed to signers_public_key?

signer/src/utxo.rs

netrome · 2024-05-07T09:18:43Z

signer/src/utxo.rs

+
+    /// Helper function for generating dummy Schnorr signatures.
+    fn generate_dummy_signature() -> Signature {
+        let key_pair = Keypair::new_global(&mut secp256k1::rand::thread_rng());


Nit: We might as well include rand as an explicit dependency since I'm sure we'll use it more times than just here.

Also, we've defaulted to using OsRng in many places in the nakamoto signer already. I don't have strong opinions on either, but it's nice to be consistent.

Cool beans, will update.

djordon · 2024-05-07T15:58:25Z

Updated! @netrome would you mind taking another look?

hstove

In general this is pretty great, but I have a question around computing the optimal tx. If what I'm talking about is known and out of scope for this PR, that totally makes sense too!

hstove · 2024-05-07T17:49:18Z

signer/src/utxo.rs

+        // Create a list of requests where each request can be approved on its own.
+        let items = deposits.chain(withdrawals);
+
+        compute_optimal_packages(items, self.reject_capacity())


I'm a bit confused as to where input/output weight come into play here. In general, inputs will be very similar size (though not exact), and outputs will be varying in size (depending on the type of address). So, wouldn't this function need to have some mechanism of knowing a request's "cost"?

Good question. The compute_optimal_packages function computes approval sets using only how the signers have voted on each of the requests. The number of rejections is the request's "weight", it is completely separate from input/output weight of a BTC transaction. That function doesn't care about the BTC weight at all.

You know, I should rename that term but I couldn't think of anything better (size? heaviness? pounds? lbs?). Everything I thought of kinda missed the mark a little but maybe weight is even worse.

netrome · 2024-05-07T18:51:50Z

Updated! @netrome would you mind taking another look?

Thanks, will do 👀

netrome

Thanks for updating, looks good to me!

netrome · 2024-05-07T15:26:01Z

signer/src/utxo.rs

+    /// The public key that will be able to spend the output.
+    pub public_key: PublicKey,


(for reference) We've synced with the team and agreed that this is a LLD bug, and that we want to use addresses here.

signer/src/utxo.rs

djordon added 3 commits May 6, 2024 17:20

Add code for constructing BTC transactions given requests

3e22949

Add a bunch of tests

2745055

Fix typos in the docs

0cd322a

djordon added the utxo consolidation Tickets related to how we consolidate deposit and withdrawal BTC transactions label May 6, 2024

djordon requested review from hstove, netrome and AshtonStephens May 6, 2024 21:50

djordon added 2 commits May 6, 2024 22:51

Update the comments and docs

bca1c87

Skip processing if there are no requests to handle

b4fa21d

netrome suggested changes May 7, 2024

View reviewed changes

Address PR comments

f9da30f

djordon mentioned this pull request May 7, 2024

[Feature]: Generate unspendable taproot addresses #130

Closed

2 tasks

Use address instead of public key

16ade11

djordon requested a review from netrome May 7, 2024 15:58

hstove reviewed May 7, 2024

View reviewed changes

netrome approved these changes May 7, 2024

View reviewed changes

djordon merged commit 205cc6f into main May 7, 2024
2 checks passed

djordon deleted the 75-construct-spend-script-btc-transactions branch May 7, 2024 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: construct BTC transactions #128

feat: construct BTC transactions #128

djordon commented May 6, 2024

netrome left a comment

netrome May 7, 2024

djordon May 7, 2024

netrome May 7, 2024

netrome May 7, 2024

djordon May 7, 2024 •

edited

Loading

netrome May 7, 2024

netrome May 7, 2024

djordon May 7, 2024

netrome May 7, 2024

netrome May 7, 2024

djordon May 7, 2024

djordon commented May 7, 2024

hstove left a comment

hstove May 7, 2024

djordon May 7, 2024

netrome commented May 7, 2024

netrome left a comment

netrome May 7, 2024

		/// The public key that will be able to spend the output.
		pub public_key: PublicKey,

		/// The public key used in the deposit script.
		pub deposit_public_key: PublicKey,

feat: construct BTC transactions #128

feat: construct BTC transactions #128

Conversation

djordon commented May 6, 2024

Description

Type of Change

Notes

Does this introduce a breaking change?

Testing information

Checklist

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon commented May 7, 2024

hstove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome commented May 7, 2024

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon May 7, 2024 •

edited

Loading